Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate iOS rust code into a single crate #6458

Merged

Conversation

pinkisemils
Copy link
Collaborator

@pinkisemils pinkisemils commented Jul 10, 2024

To simplify (and speedup) the build process for the iOS app, the FFI for all of our rust code will now be accessed through a single crate and a single static library. This also enables the app to use a single tokio runtime per process.


This change is Reviewable

@pinkisemils pinkisemils requested review from hulthe and acb-mv July 10, 2024 13:22
Copy link

linear bot commented Jul 10, 2024

@pinkisemils pinkisemils force-pushed the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch 3 times, most recently from d546966 to f860ec0 Compare July 11, 2024 11:34
Copy link
Contributor

@acb-mv acb-mv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 56 of 56 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


ios/MullvadRustRuntime/include/mullvad_rust_runtime.h line 101 at r1 (raw file):

                                         const uint8_t *raw_preshared_key,
                                         const uint8_t *raw_ephemeral_private_key);

A minor suggestion: maybe add a barrier comment saying that this is where shadowsocks begins, i.e., something like:

Code snippet:

//
// --- Shadowsocks
//

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 56 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


ios/MullvadRustRuntime/include/mullvad_rust_runtime.h line 1 at r1 (raw file):

#include <stdarg.h>

Is this file generated by mullvad-ios/build.rs? Could be worth adding a "warning, autogenerated" header here.

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pinkisemils)


mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 89 at r1 (raw file):

    pub fn run(self) {
        let runtime = mullvad_ios_runtime().expect("Could not get tokio runtime");

This is probably waaay out of scope but we might want to check so we don't panic unwind anywhere across the ffi.

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 26 of 56 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pinkisemils)


mullvad-ios/src/shadowsocks_proxy/bin/run.rs line 1 at r1 (raw file):

use std::{net::SocketAddr, str::FromStr};

I thought bins had to be placed in /src/bin?

@pinkisemils pinkisemils force-pushed the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch from f860ec0 to 665f3f1 Compare July 16, 2024 13:08
Copy link
Collaborator Author

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 45 of 56 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @hulthe)


ios/MullvadRustRuntime/include/mullvad_rust_runtime.h line 1 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Is this file generated by mullvad-ios/build.rs? Could be worth adding a "warning, autogenerated" header here.

It is auto-generated, and thus it might be a pain to add that header since it's regenerated on every build. The only reason it is in git is because Xcode won't attempt to run the build if the header isn't even there.


ios/MullvadRustRuntime/include/mullvad_rust_runtime.h line 101 at r1 (raw file):

Previously, acb-mv wrote…

A minor suggestion: maybe add a barrier comment saying that this is where shadowsocks begins, i.e., something like:

Since it is autogenerated, the comments are lifted straight from the rust code.


mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 89 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

This is probably waaay out of scope but we might want to check so we don't panic unwind anywhere across the ffi.

It really is not out of scope at all. I've changed it so that the FFI call returns an error early, and the handle to the runtime is passed down here.


mullvad-ios/src/shadowsocks_proxy/bin/run.rs line 1 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I thought bins had to be placed in /src/bin?

This is a test binary, we probably don't want it here at all.

@pinkisemils pinkisemils force-pushed the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch from 665f3f1 to db6ce8b Compare July 16, 2024 14:01
@hulthe hulthe requested a review from acb-mv July 16, 2024 14:07
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 7 of 56 files at r1, 11 of 11 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acb-mv)


mullvad-ios/src/post_quantum_proxy/ios_runtime.rs line 89 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It really is not out of scope at all. I've changed it so that the FFI call returns an error early, and the handle to the runtime is passed down here.

Nice, elegant fix.

@pinkisemils pinkisemils force-pushed the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch 2 times, most recently from c5ecd6c to e3ad395 Compare July 17, 2024 09:47
@pinkisemils pinkisemils force-pushed the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch from e3ad395 to 2df280f Compare July 17, 2024 09:48
@pinkisemils pinkisemils merged commit 0ba177b into main Jul 17, 2024
52 of 53 checks passed
@pinkisemils pinkisemils deleted the reuse-the-same-tokio-runtime-in-packettunnel-ios-596 branch July 17, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants